Add signature verification to gossip by sagar-solana · Pull Request #1937 · solana-labs/solana

sagar-solana · 2018-11-28T01:22:37Z

Problem

Gossip lacks signature verification for messages, which means data can be spoofed.

Summary of Changes

Added Signatures to replicated gossip values and added verification of values are they get replicated across the network

Fixes #1855 and #1952

garious · 2018-11-28T16:56:11Z

    /// The network
    pub gossip: CrdsGossip,
+    /// set the keypair that will be used to sign crds values generated. It is unset only in tests.
+    keypair: Option<Arc<Keypair>>,


I don't see the need for the Option here. Instead of None, can you use Pubkey::default()?

Yeah, Rob was saying the same. I think it makes a little more sense for clarity sake. I'll explain and maybe you can suggest a better solution to the overall problem.

Cluster Info uses the pubkey from the node_info which gets passed in during construction. In fullnodes (most common production usecase), the node_info's pubkey is the same as the keypair that gets passed in optionally(via the new constructor I added).
In nearly all other uses (spy nodes, tests etc) cluster info just carrying around dummy nodes who's pubkeys are no longer associated with any keypair that cluster info can use for signature verification. If I init cluster's keypair to default, it will always fail verification during gossip. The same failure occurs if keypair isn't even initialized since the messages won't be signed and their signatures will not verify. So given that the end result is the same, I chose the Option to make it a little clearer why verification might be failing. Having a valid keypair makes it seem like the node's pubkey is the same as the keypair's but that's not necessary and that's the actual problem here. Cluster info can be constructed with nodes and not have a valid keypair to sign messages with.

Optionally, I can ignore the node's pubkey and just use the keypair always but then that becomes a special case for gossip where gossip messages are always signed with a random keypair whose pubkey doesn't tie into any of the other info blobs (nodes) in the system.

@sagar-solana have ClusterInfo::new(..) take a keypair and get rid of default?

@aeyakovenko, yup I can do that but it doesn't solve the problem that most users of cluster info just give it a random node whose keypair is discarded. What keypair do I pass in those cases? In our primary usecase (fullnode's instance of cluster which gets passed to ncp) this isn't an issue but in a whole bunch of places we don't have a keypair to pass in.

Unless its totally fine to expect users of cluster_info to pass in random keypairs knowing that gossip will fail in that cluster_info since that's what will happen with how I have it right now if cluster_info is constructed without a keypair. In that case I'd be more than happy to get rid of default.

Is it looking like a ton of work to refactor such that all tests are using legit keypairs?

Pretty much. Most tests don't even care about having gossip run so it would be sort of unnecessary as I see it right now. For now I've done what you asked and removed the option but left in the other constructor. All Fullnodes should work correctly with gossip verification. If anything else tries to run gossip the peers will drop their messages (like spynodes shouldn't be gossiping anyway, only listening) so that actually works out well.

aeyakovenko · 2018-11-29T01:49:25Z

    pub fn is_valid_address(addr: &SocketAddr) -> bool {
        (addr.port() != 0) && Self::is_valid_ip(addr.ip())
    }
+    pub fn get_sign_data(&self) -> Vec<u8> {


@sagar-solana can you break this out into a singe signed serialized structure? @garious i am not a huge fan of this pattern from Transaction, because of how error prone it is.

Yes, Transaction should have a TransactionData struct that includes everything but its signatures.

Alright I can. I'll end up sticking signature into the Protocol enums instead of into the individual crdsvalues. That way I can just use default serialization for the values. Sounds good? Or should I do what @garious suggested with a data inner struct?

@sagar-solana the protocol enums wont work because when you receive a list of CrdsValues you need to verify the signature of each one. The protocol messages are the p2p messages, but the values themselves propagate p2N

Oh that's right! Okay then I need to use the inner data structs to avoid manually serializing. Thanks for pointing that out. Got carried away trying to simplify...

rob-solana · 2018-11-30T23:20:18Z

+    }
+
+    fn pubkey(&self) -> Pubkey;
+    fn get_sign_data(&self) -> Vec<u8>;


naming nit... getters don't have get_ on them in our conventions

I didn't come up with that name. Txs already use it :P Suggestions? Just sign_data (could be confused with "please sign this data")?

could be "signed_data()" ?

But the is_signed state of this data isn't known. It's the data that gets signed and compared against for verification. It's not already signed. :(

signable_data() to match the trait?

yup, awesome. Thanks!

rob-solana · 2018-11-30T23:24:23Z

 use solana_sdk::pubkey::Pubkey;

+///The min size for bloom filters
+pub const CRDS_GOSSIP_BLOOM_SIZE: usize = 1000;


is this in bits? does it make sense for it to be a multiple of 8?

Not in bits. Num bits to use is figured out using this value (usually network_size is passed in). See https://hur.st/bloomfilter/.

- Assign default to keypair

- Needs tests

- Added a Signable trait that all signable values can implement. - CrdsValue is a Signable that contains Signables so it doesn't need to provide implementations for all the methods and can just override some defaults

- Spy nodes should be able to make pull requests

sagar-solana · 2018-12-01T03:56:54Z

@aeyakovenko, if this looks ok to you, I'll merge.

aeyakovenko · 2018-12-01T06:19:48Z

+    /// The Pubkey of the intended node/destination for this message
+    pub destination: Pubkey,
+    /// The source Addr this message should have been received from
+    pub source: SocketAddr,


The source shouldn't be necessary, and might not be possible to enforce because of NATs. I would add a wallclock to the message so it expires if its replayed at a later time or by a random node.

Oh I didn't think of that. The wall clock's a good idea though.

@aeyakovenko, I picked 500ms as the timeout. Does that sound reasonable since prunes are p2p?

aeyakovenko

500ms might be tight, but we need to tests all the constants in the real world. Could you add a counter for expired prune messages? (A separate pr to track behavior is fine)

Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.3.3 to 4.3.4. - [Release notes](https://github.com/Microsoft/TypeScript/releases) - [Commits](microsoft/TypeScript@v4.3.3...v4.3.4) --- updated-dependencies: - dependency-name: typescript dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…port of solana-labs#1888) (solana-labs#1900) (solana-labs#1937) Revert "v2.0: Refactor and additional metrics for cost tracking (backport of solana-labs#1888) (solana-labs#1900)" This reverts commit 0aef62e.

sagar-solana added the work in progress This isn't quite right yet label Nov 28, 2018

garious reviewed Nov 28, 2018

View reviewed changes

Comment thread src/cluster_info.rs Outdated

sagar-solana added the testnet label Nov 28, 2018

solana-grimes removed the testnet label Nov 28, 2018

sagar-solana added the noCI Suppress CI on this Pull Request label Nov 29, 2018

aeyakovenko reviewed Nov 29, 2018

View reviewed changes

sagar-solana added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request work in progress This isn't quite right yet labels Nov 30, 2018

solana-grimes removed the CI Pull Request is ready to enter CI label Nov 30, 2018

sagar-solana added the testnet label Nov 30, 2018

solana-grimes removed the testnet label Nov 30, 2018

sagar-solana requested a review from rob-solana November 30, 2018 22:20

rob-solana reviewed Nov 30, 2018

View reviewed changes

rob-solana approved these changes Nov 30, 2018

View reviewed changes

sagar-solana added 12 commits November 30, 2018 19:13

Add signatures to crds values

5fff269

Add Signature verification to gossip messages

abc3743

Fix fmt

4e47f42

Add test for gossip msg verification

da9f334

- Assign default to keypair

fmt fix

ba53e74

Add verification to prune messages

51ed099

Add source addr verification

9b88cd2

- Needs tests

Add Signable trait

8b8f3f9

- Added a Signable trait that all signable values can implement. - CrdsValue is a Signable that contains Signables so it doesn't need to provide implementations for all the methods and can just override some defaults

Add a test to check the prune messages

4622fa4

Remove verification from pull reqs

1989a30

- Spy nodes should be able to make pull requests

Update min bloom filter size and update tests

75c3a0f

Remove unused

f0ea5bb

sagar-solana added 3 commits November 30, 2018 19:13

Fix test to use consistent keypair

309bd88

Fix fmt

4b85c40

Rename get_sign_data to signable_data

bab48f4

sagar-solana force-pushed the gossip_signing branch from 5f5e1f7 to bab48f4 Compare December 1, 2018 03:19

aeyakovenko reviewed Dec 1, 2018

View reviewed changes

Add wallclock to prune data

92d6ea9

aeyakovenko approved these changes Dec 1, 2018

View reviewed changes

Fix fmt

cc601d7

sagar-solana merged commit 34c3a0c into solana-labs:master Dec 1, 2018

sagar-solana deleted the gossip_signing branch December 1, 2018 20:00

Conversation

sagar-solana commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagar-solana Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeyakovenko Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagar-solana Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob-solana Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagar-solana commented Dec 1, 2018

Uh oh!

aeyakovenko Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeyakovenko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sagar-solana commented Nov 28, 2018 •

edited

Loading

sagar-solana Nov 28, 2018 •

edited

Loading

aeyakovenko Nov 29, 2018 •

edited

Loading

sagar-solana Nov 30, 2018 •

edited

Loading

rob-solana Nov 30, 2018 •

edited

Loading

aeyakovenko Dec 1, 2018 •

edited

Loading